Manchester| 26-ITP-Jan | fonime Edak | Sprint 2 | Data-groups#1110
Manchester| 26-ITP-Jan | fonime Edak | Sprint 2 | Data-groups#1110Ofonimeedak wants to merge 7 commits intoCodeYourFuture:mainfrom
Conversation
Ofonimeedak
added
📅 Sprint 2
cjyuan
added
Reviewed
Ofonimeedak
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
4 of the inline comments in my previous review were hidden, and you probably missed them. Can you also address those comment?
cjyuan
removed
the
Needs Review
|
What about the 4 unaddressed comments in the first review? |
|
I searched I thought it was the one on look up.js that I have addressed |
Sprint-2/implement/querystring.js
Outdated
| if (queryString.length === 0) { | ||
| return queryParams; | ||
| } | ||
| if (typeof queryString !== "string" || !queryString.includes("=")) |
There was a problem hiding this comment.
!queryString.includes("=")
This check would reject key1&key2&key3 but accept key1=&key2&key3.
There was a problem hiding this comment.
Thank you for this edge case. It got me thinking 2 things: whether I should consider such input as invalid, or I should return an empty object literal.
I had to consider the latter to keep the loop running
There was a problem hiding this comment.
You should change your code to meet your expectation (instead of the other way around). Since the spec wasn't clear how the function is expected to handle empty key or value, you code is good.
Ofonimeedak
added
the
Needs Review
cjyuan
added
Complete
2 participants
Self checklist
This PR contains fixed bug codes with test cases for sprint 2
Please kindly find this as the link to my closed pull request for all commits
#1102